-
Notifications
You must be signed in to change notification settings - Fork 24
Add infra pytest warnings plugin #2757
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev-milestone
Are you sure you want to change the base?
Add infra pytest warnings plugin #2757
Conversation
dpnp/tests/infra_warning_utils.py
Outdated
| os.makedirs(self.directory, exist_ok=True) | ||
| self._events_file = os.path.join(self.directory, self.events_artifact) | ||
| self._events_fp = open( | ||
| self._events_file, | ||
| "w", | ||
| encoding="utf-8", | ||
| buffering=1, | ||
| newline="\n", | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be safe and clear to use from pathlib import Path and then:
| os.makedirs(self.directory, exist_ok=True) | |
| self._events_file = os.path.join(self.directory, self.events_artifact) | |
| self._events_fp = open( | |
| self._events_file, | |
| "w", | |
| encoding="utf-8", | |
| buffering=1, | |
| newline="\n", | |
| ) | |
| p = Path(self.directory).expanduser().resolve() | |
| if p.exists() and not p.is_dir(): | |
| raise ValueError(f"{p} exists and is not a directory") | |
| p.mkdir(parents=True, exist_ok=True) | |
| if Path(self.self.events_artifact).name != self.events_artifact: | |
| raise ValueError(f"Invalid filename (must not contain path parts): {self._events_file}") | |
| self._events_file = p / self.events_artifact | |
| self._events_fp = self._events_file.open(mode="w", encoding="utf-8", buffering=1, newline="\n") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good suggestion that works safe across platforms. I have adopted you suggestion.
Since this is a public recipe, my agenda here is to not raise error and break the workflow because of this plugin for what ever the reason. This would rather print and artifact wont be created.
a337f4d to
b209cab
Compare
| except Exception as exc: | ||
| self._events_fp = None | ||
| self._events_file = None | ||
| self.directory = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It holds the env variable and it was set during the init. Why do we want to reset that here?
| EVENT_PREFIX = "DPNP_WARNING_EVENT - " | ||
|
|
||
| def __init__(self): | ||
| self.enabled = bool(warn_config.infra_warnings_enable) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to cast, already holds a boolean value:
| self.enabled = bool(warn_config.infra_warnings_enable) | |
| self.enabled = warn_config.infra_warnings_enable |
| except Exception: | ||
| pass | ||
| self._events_fp = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since we don't have any special exception handling:
| except Exception: | |
| pass | |
| self._events_fp = None | |
| finally: | |
| self._events_fp = None |
This PR introduce the plugin feature to capture and format pytest warnings to process it in internal CI.
These changes will not add any overhead to existing pytest scope. This feature can be fully enabled/disabled by env var -
DPNP_INFRA_WARNINGS_ENABLE